Skip to content

Comments

Support for Trill capacitive sensors#657

Open
giuliomoro wants to merge 7 commits intoelectro-smith:masterfrom
giuliomoro:master
Open

Support for Trill capacitive sensors#657
giuliomoro wants to merge 7 commits intoelectro-smith:masterfrom
giuliomoro:master

Conversation

@giuliomoro
Copy link

@giuliomoro giuliomoro commented Nov 23, 2024

This is the "Linux" library (available here) wrapped into libDaisy. The low-level I2c I/O stuff is done here: https://github.com/giuliomoro/libDaisy/blob/64c08470c079693cd9e2ab59cf2b85a5cff4727c/src/dev/trill/I2c.h

Contributions by me and @virvel .
Tested by @dromer and @virvel when it was based on 17ee39b.
Previous discussion giuliomoro#1

Closes #602

used:

clang-format-11 --verbose -i src/dev/trill/*{.cpp,.h} examples/Trill*/*{.cpp.h}
@giuliomoro
Copy link
Author

OK it's now brought up to date with the latest upstream and I even figured out how to do the formatting. @virvel @dromer I'd appreaciate if you have a chance to test this again.

@virvel
Copy link

virvel commented Nov 24, 2024

OK it's now brought up to date with the latest upstream and I even figured out how to do the formatting. @virvel @dromer I'd appreaciate if you have a chance to test this again.

I can confirm that this works!

@stephenhensley
Copy link
Collaborator

Hi y'all! This is looking great!
Thanks for the huge contribution!

While I think there is quite a bit that would make this feel more like the rest of the libDaisy, but I'd be happy to merge it with a few minor tweaks.

At a minimum I think we'll want to:

  1. The Trill object should be added to the daisy namespace instead of being global
  2. I'd suggest that we also wrap most/all of the internals that have been added to support the Trill object in a daisy::trill namespace (this is to help avoid things like trill's I2c from being included in the global or daisy namespace and causing any confusion.
  3. the /src/dev/trill/Trill.h file should be included in daisy.h so user's don't need to manually include the path (the examples could then be updated as well)

From a style/consistency perspective, there are a few more changes that would affect the API if they're changed later:

  • class methods in libDaisy typically start with a capital letter instead of camel-case (e.g. Trill:readI2C() -> Trill::ReadI2C()
  • the initialization class method in libDaisy is typically called, Init()
  • typically the Init method takes a Config struct with any settings the user might provide
    • Reason for this is to provide a more stable user API while allowing drivers to add features/additional configuration without changing how the user interacts with the object.

Since this would be a new device, I'm open to the API changing a bit over time. So the bullet-list items aren't strictly necessary at this time, but would be welcome at any point.

However, I would like for the initial few points to be addressed before we merge it.


I don't foresee any of the recent/upcoming changes having an impact on your PR. So no rush.
I had honestly thought I left a comment similar to this several months ago, but I must not have finished writing it or something.

Thanks again for the contribution, and let me know if you any questions!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Feature: Trill sensor support

3 participants